inject balances from json into chainspec #3114

dariolina · 2024-10-10T11:39:54Z

It is required to keep the chain spec file manageable with thousands of balances. The JSON file is NOT final as it only contains farmer rewards and no other allocations (team, fdn, etc.).
Tested on devnet on Oct 9.

Code contributor checklist:

I have read, understood and followed contributing guide

nazar-pc

Are you sure we need to include this in the software in JSON? It'll make every node build ~12M larger. Compressing it with zstd will reduce the size to ~4M for example.

nazar-pc · 2024-10-10T19:44:28Z

crates/subspace-node/src/genesis_allocations.rs

+use sp_runtime::traits::Zero;
+
+pub fn get_genesis_allocations() -> Vec<(AccountId, Balance)> {
+    let file_content = include_str!("genesis_allocations.json");


We typically store such things in a constant at the top of the file, not in local variable, see the chain spec example. Also I'd make this function take contents as an argument with constant that contains JSON being in the chain spec file.

nazar-pc · 2024-10-10T19:46:12Z

crates/subspace-node/src/genesis_allocations.rs

+    let allocations: Value = serde_json::from_str(file_content)
+        .expect("Failed to parse genesis allocations JSON");
+
+    allocations.as_array()


All of these types implement Serde. You can just define the shape and then decode the whole thing without manually iterating over it, for balances you can use NonZeroU128 to automatically check the invariant.

That is unless you want to have nice error messages here (that should never really happen anyway).

teor2345

Are you sure we need to include this in the software in JSON? It'll make every node build ~12M larger. Compressing it with zstd will reduce the size to ~4M for example.

Can we reduce the file size using arrays rather than key/value maps?
That would look like:

[
  [
    "stBJ4naCdaku9nTXrcQ8m8XQSa4arro9pSdW3eaj6skgW9Nwg",
    "1000000000000000000000"
  ],
  [
    "st9idRv3z62dVwfmjLaNiq1FVb2B2j3giib7ns4Vni1wZ9ePj",
    "1106022470000000000000000"
  ],
  ...
]

It also seems like all the amounts end in 15 zeroes, so maybe we could do that multiplication in code?

If this large file isn't final, then compressing it will also add 4 MB to the Git repository every time it changes. If we are going to compress it, there's no need for so much white space, so we could run it through a script that removes all white space before compressing it.

If it's uncompressed, it will add 12 MB to Git, but the diffs will be smaller.

Is there a way to just store the file hash in the node/git, and download the latest file when the node starts? Or distribute the file with the node build?

nazar-pc · 2024-10-10T23:02:39Z

Is there a way to just store the file hash in the node/git, and download the latest file when the node starts? Or distribute the file with the node build?

This is supposed to be a part of the chain spec, so it will be awkward to distribute it separately and I'd really prefer to not have it depend on the download from somewhere.

At the same time this large blob will end up copied into the actual mainnet chain spec, which in turn means our chain spec will be permanently huge. So we will remove the file shortly afterwards, but the ballooned chain spec will stay with us basically forever.

Repo size is a smaller concern for me, this is a really important information to have persisted in history. But I agree that we should wait for the very final version rather than adding multiple revisions of this large file.

dariolina · 2024-10-11T08:32:45Z

I have added the file, even if not final, to test the "real deal" and wanted us to align on a preferred way to handle such case, because that is the size of the file we will be dealing with on mainnet. If we decide on format/workflow here, in the next PR I can just swap the allocations file for the final one.
There is no way to avoid ballooned mainnet chainspec, however, these are not strictly necessary for testnet, so we could save some there.

NingLin-P · 2024-10-11T08:38:27Z

Perhaps adding the file to a separate repro and using it as git submodule can help to reduce the mono repo size.

nazar-pc · 2024-10-11T12:19:14Z

Perhaps adding the file to a separate repro and using it as git submodule can help to reduce the mono repo size.

It really doesn't if you need to clone submodule for code to compile anyway 🙂

dariolina · 2024-10-11T14:37:29Z

It also seems like all the amounts end in 15 zeroes, so maybe we could do that multiplication in code?

In the final file it will be closer to 8 trailing zeroes, but I would rather not introduce into the code assumptions about precision of underlying file.

teor2345 · 2024-10-13T20:30:45Z

There is no way to avoid ballooned mainnet chainspec, however, these are not strictly necessary for testnet, so we could save some there.

I think changing to arrays and removing spaces will help keep the size of the file down. (I don’t want to remove new lines, because that makes diffs much harder to see.)

Is there anything we need to test on testnet that would be missed by having a small chain spec file?
It’s usually better to keep them as similar as possible, unless we’re testing mainnet the same as testnet before every release. (I’ve run into mainnet-only bugs in other distributed networks before.)

teor2345 · 2024-10-13T20:34:42Z

crates/subspace-node/src/chain_spec.rs

@@ -86,7 +87,7 @@ pub fn gemini_3h_compiled() -> Result<GenericChainSpec, String> {
            AccountId::from_ss58check("5DNwQTHfARgKoa2NdiUM51ZUow7ve5xG9S2yYdSbVQcnYxBA")
                .expect("Wrong root account address");

-        let balances = vec![(sudo_account.clone(), 1_000 * SSC)];
+        let balances=get_genesis_allocations(GENESIS_ALLOCATIONS);


This should make rustfmt happier

Suggested change

let balances=get_genesis_allocations(GENESIS_ALLOCATIONS);

let balances = get_genesis_allocations(GENESIS_ALLOCATIONS);

dariolina · 2024-10-14T08:24:47Z

(I don’t want to remove new lines, because that makes diffs much harder to see.)

In the next iteration, the file will change basically on every line since the precision will be higher and post-merge there should not ever be a diff to genesis balances.

teor2345 · 2024-10-14T08:26:57Z

(I don’t want to remove new lines, because that makes diffs much harder to see.)

In the next iteration, the file will change basically on every line since the precision will be higher and post-merge there should not ever be a diff to genesis balances.

Sure, we can always run it through jq to format and then do a diff. So whatever you want to do to minify is fine with me!

inject balances from json into chainspec

9a739ce

dariolina requested a review from jfrank-summit October 10, 2024 11:39

dariolina requested review from NingLin-P, nazar-pc and rg3l3dr as code owners October 10, 2024 11:39

nazar-pc reviewed Oct 10, 2024

View reviewed changes

teor2345 reviewed Oct 10, 2024

View reviewed changes

use serde to read allocations json

e862cee

teor2345 reviewed Oct 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inject balances from json into chainspec #3114

inject balances from json into chainspec #3114

dariolina commented Oct 10, 2024

nazar-pc left a comment

nazar-pc Oct 10, 2024

nazar-pc Oct 10, 2024

teor2345 left a comment

nazar-pc commented Oct 10, 2024

dariolina commented Oct 11, 2024

NingLin-P commented Oct 11, 2024

nazar-pc commented Oct 11, 2024

dariolina commented Oct 11, 2024 •

edited by nazar-pc

Loading

teor2345 commented Oct 13, 2024

teor2345 Oct 13, 2024

dariolina commented Oct 14, 2024

teor2345 commented Oct 14, 2024

	let balances=get_genesis_allocations(GENESIS_ALLOCATIONS);
	let balances = get_genesis_allocations(GENESIS_ALLOCATIONS);

inject balances from json into chainspec #3114

Are you sure you want to change the base?

inject balances from json into chainspec #3114

Conversation

dariolina commented Oct 10, 2024

Code contributor checklist:

nazar-pc left a comment

Choose a reason for hiding this comment

nazar-pc Oct 10, 2024

Choose a reason for hiding this comment

nazar-pc Oct 10, 2024

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

nazar-pc commented Oct 10, 2024

dariolina commented Oct 11, 2024

NingLin-P commented Oct 11, 2024

nazar-pc commented Oct 11, 2024

dariolina commented Oct 11, 2024 • edited by nazar-pc Loading

teor2345 commented Oct 13, 2024

teor2345 Oct 13, 2024

Choose a reason for hiding this comment

dariolina commented Oct 14, 2024

teor2345 commented Oct 14, 2024

dariolina commented Oct 11, 2024 •

edited by nazar-pc

Loading